fix(pg): the readiness mount check sees a router Sentry has wrapped - #1958
Conversation
Deploy 4e60f24 (#1901) hung at the backend rollout. The new pod connected to Postgres and mounted /api/pg/messages (it answered 401), but /api/health/ready said "PostgreSQL routes are not mounted on this pod" and the pod never became Ready. routerIsMounted compared layer.handle === router, and @sentry/node 10.65, initialised in production, wraps every Express layer handle. Reproduced locally with the backend's own modules: with Sentry initialised the identity check is false and the router's stack is still the same array; without Sentry both are true. The check now also accepts a layer whose handle exposes the router's own stack array. That is still an identity question about this router, not a path match. Two tests build a real Express app and wrap the layer the way Sentry does: the mounted router is found, and a different wrapped router is not. The first is red on main's pgBootService. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUTWqLu6zhFPraoPHZcjQP
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
CODE GATE: PASS @ 136530ee — sprint-review. Behind 0, author Lily, 2 files / +47.
I gated #1901, whose identity comparison is the defect, so I checked the premise against the real library rather than this PR's stand-in for it.
Real @sentry/node 10.65.0, plain node, Sentry.init() before require('express'): the mounted layer's handle is a function named layerHandlePatched; handle === router is false; handle.stack === router.stack is true. The reproduction in the description holds against the actual library, not just against wrapLikeSentry.
The decisive run — real Sentry, plain node (the production module-loading path), the real routerIsMounted required from each tree:
| tree | mounted router | unmounted | wrong router |
|---|---|---|---|
main e6a8252 |
false ← the incident | false | false |
| head 136530e | true | false | false |
The incident reproduces against main's code under real instrumentation, this fix resolves it, and it introduces no false positive. Array identity is the correct key: only a wrapper of that router can hold its stack object, so the clause cannot alias a different router.
Mutation, anchor count 1 (no line-scoping needed):
- BASE — 15/15.
- Delete the new clause —
finds the mounted router when instrumentation has wrapped its layer handlereddens. 1 failed / 15. - Invert to
!==—does not mistake a different wrapped router for this onereddens. 1 failed / 15. - RESTORED — 15/15,
git diff --quietclean.
The inversion mutant is the useful one: it shows the second test is not redundant with the first. It pins false-positive safety rather than mere presence, which is exactly the property a .stack comparison needs.
PR tests run against main's pgBootService: exactly one red, the found-case — the claim in the description is confirmed. Collateral: 45/45 across pgBootService, routes/health, config/db-pg. CI has nothing red; Test & Coverage is still running, which is the whole of the BLOCKED mergeStateStatus.
One structural limit worth recording. Not a blocker, and not a criticism of the approach.
The unit tier cannot host real Sentry. I ran this suite's scenario with a genuine Sentry.init() under jest and got handle === router true — no wrapping at all. OTel's require-in-the-middle hook never sees express through jest's module registry. So wrapLikeSentry isn't a shortcut here; it is the only thing available at this tier, and hand-rolling it was right.
The consequence is what matters: these tests pin the fix, not the premise. They assert that routerIsMounted handles a wrapper shaped the way we believe Sentry's is. Nothing in CI asserts that Sentry still produces that shape. If a Sentry upgrade stops exposing .stack on the wrapper, all 15 stay green and /ready starts lying again in exactly the way it just did — with the same signature, a pod serving 401 on /api/pg/messages while reporting "not mounted".
That gap is cheap to close with the ~20-line plain-node probe I used for the table above: real Sentry.init(), real express, assert handle !== router && handle.stack === router.stack. It belongs in a follow-up as a script, not in this P0.
|
Correction to my gate above — the "structural limit" section overstated the consequence. The verdict (PASS @ I wrote that if a Sentry upgrade stops exposing
So on a recurrence: The residual gap is narrower than I said, and differently shaped than "timing." The chat-check step at In a recurrence the two gates would disagree in the informative direction: the rollout gate red because readiness is lying, while the pod genuinely serves 401 because the routes really are mounted. But the step whose error text spells out So the residual is timing plus diagnostic specificity, and both are worth closing:
Nothing here blocks this PR; it is still PASS. Recording it because the claim was published in this thread, and a retraction that stays in chat does not reach anyone reading the review. |
The premise guard's suite is red on main by construction — main's pgBootService has no stack clause — so it was held until #1958 merged. Now the tree under test carries the fix the guard pins.
1 participant
P0 for deploys. Deploy 4e60f24 (#1901) hung at the backend rollout, and every backend deploy will hang the same way until this lands.
The new pod connected to Postgres and mounted
/api/pg/messages, which answered 401. But/api/health/readysaid "PostgreSQL routes are not mounted on this pod", so the pod never became Ready. Production stayed safe: the old pod kept serving, and the frontend rolled out.Cause.
routerIsMountedcomparedlayer.handle === router, and@sentry/node10.65, initialised in production (SENTRY_DSNis set), wraps every Express layer handle.Reproduced locally with the backend's own modules:
stackstill the same arrayFix. The check also accepts a layer whose handle exposes the router's own
stackarray. That is still an identity question about this router, not a path match.Tests. Two build a real Express app and wrap the layer the way Sentry does:
pgBootService.test.jspasses; eslint ran through lint-staged.server.test.jsfails locally with a Node 26jsonwebtokenload error, identically on main, so CI is the reference for it.Gate: sprint-review, who gated #1901.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RUTWqLu6zhFPraoPHZcjQP